refactor(app-router): extract app page dispatch by NathanDrake2406 · Pull Request #986 · cloudflare/vinext

NathanDrake2406 · 2026-04-30T05:26:13Z

What changed

This extracts App Router page request orchestration out of the generated RSC entry and into packages/vinext/src/server/app-page-dispatch.ts.

The generated entry now describes app shape and request wiring only: matched route, params, component tree builder, boundary render callbacks, middleware context, cache keys, and module loaders. The normal runtime module owns behavior: method policy, force-static/error setup, ISR cache read/regeneration, dynamic params validation, intercepting route responses, page element build error handling, lifecycle render delegation, and special-error fallback routing.

Why

This follows the principle that codegen should describe the app shape while normal modules implement behavior. The prior generated template mixed route-specific imports with a large page dispatch pipeline, which made orchestration harder to type, test, review, and evolve independently from route manifest generation.

Behavior tests

Added tests/app-page-dispatch.test.ts with response-level behavior coverage for the new dispatch module:

production page cache HIT serves cached HTML instead of revalidating params or rendering
unsupported page methods return the method policy response instead of rendering
dynamicParams = false returns 404 for paths outside generated params
intercepted RSC source-route payloads preserve middleware status and headers

These tests assert HTTP-visible outcomes: status, headers, and response body. They avoid asserting dispatch call counts or generated implementation details.

Next.js references

Next.js uses a similar separation between generated app-page shape and runtime page rendering modules:

Validation

vp check --fix knip.ts packages/vinext/src/entries/app-rsc-entry.ts packages/vinext/src/server/app-page-dispatch.ts packages/vinext/src/server/app-page-request.ts tests/app-page-dispatch.test.ts tests/app-router.test.ts tests/entry-templates.test.ts tests/__snapshots__/entry-templates.test.ts.snap
vp test run tests/app-page-dispatch.test.ts
vp test run tests/app-page-dispatch.test.ts tests/entry-templates.test.ts tests/app-router.test.ts tests/app-page-render.test.ts tests/app-page-request.test.ts tests/app-page-cache.test.ts
vp run knip --no-progress
Commit hook reran staged-file checks and Knip successfully.

Risk coverage

Covered the main behavioral risks through dedicated dispatch behavior tests plus the App Router integration suite: dev/prod App Router rendering, RSC/HTML responses, ISR cache reads and writes, route handler separation, middleware header propagation, dynamic params validation, intercepting routes, not-found/forbidden/unauthorized boundaries, and generated-entry delegation assertions.

The generated-code tests intentionally assert only the delegation contract now. Runtime dispatch behavior is covered by tests/app-page-dispatch.test.ts, existing helper tests, and the App Router integration suite.

pkg-pr-new · 2026-04-30T05:26:53Z

Open in StackBlitz

npm i https://pkg.pr.new/vinext@986

commit: 6938714

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

james-elicx · 2026-04-30T19:04:31Z

/bigbonk review

ask-bonk

Clean extraction that follows the stated architecture principle: generated entries describe app shape, normal modules own behavior.

What I checked

Behavioral fidelity: The dispatch module reproduces the same pipeline order as the old inline code: fetch soft tags → default-export check → method policy → force-static/dynamic-error context setup → ISR cache read → dynamic params validation → intercept resolution → page element build → render lifecycle. No steps reordered or dropped.
setCurrentFetchSoftTags timing improvement: The old entry called setCurrentFetchSoftTags with a ternary (route.routeHandler ? "route" : "page") before the route-handler/page branch. The new code correctly tags route handlers with "route" in the entry (line 1125) and pages with "page" in dispatch (line 282). Same net behavior but cleaner separation.
Import resolution: All imports in app-page-dispatch.ts resolve to existing modules. createAppPageTreePath is exported from app-page-route-wiring.tsx:153. createRequestContext/runWithRequestContext in the dispatch module are the same functions the entry previously aliased as _createUnifiedCtx/_runWithUnifiedCtx.
Type safety at the seam: The DispatchAppPageOptions type is well-structured — route shape is constrained by TRoute extends AppPageDispatchRoute, all callbacks are properly typed, and optional fields (debugClassification, isrDebug, scriptNonce, mountedSlotsHeader) correctly use ?.
Test coverage: The ~170 deleted test lines in app-router.test.ts were generated-code string assertions (checking that __readAppPageCacheResponse, __validateAppPageDynamicParams, __renderAppPageLifecycle appeared in the template). These are replaced by: (1) the new entry-templates.test.ts assertions verifying the delegation contract (dispatchAppPage as __dispatchAppPage, return __dispatchAppPage({), and (2) the new app-page-dispatch.test.ts with response-level behavioral tests. No behavioral coverage lost.
CI: All checks green — Check, Vitest (unit + 3 integration shards), all 10 E2E projects.

Minor observations (non-blocking)

Two small things noted inline. Neither blocks merge.

ask-bonk · 2026-04-30T19:08:47Z

+  extraTags: string[],
+  routeSegments: readonly string[],
+): string[] {
+  return buildPageCacheTags(cleanPathname, extraTags, [...routeSegments], "page");


Nit: buildPageCacheTags takes routeSegments: string[] (mutable array), but AppPageDispatchRoute.routeSegments is readonly string[]. The spread [...routeSegments] here works correctly to satisfy the type, but createAppPageTreePath on line 531 does the same spread. If buildPageCacheTags and createAppPageTreePath accepted readonly string[] instead, these defensive copies wouldn't be needed. Not blocking — just noting the pattern for a future cleanup pass.

ask-bonk · 2026-04-30T19:08:47Z

+import { buildPageCacheTags } from "./implicit-tags.js";
+import type { ISRCacheEntry } from "./isr-cache.js";
+
+type AppPageParams = Record<string, string | string[]>;


Several type aliases here (AppPageParams, AppPageBoundaryOnError, AppPageDebugLogger, AppPageCacheSetter, AppPageCacheGetter) are duplicated from app-page-render.ts (lines 35-46). These could be shared from a common types module to avoid drift. Non-blocking since they're simple types unlikely to diverge, but worth noting for a future types consolidation pass.

ask-bonk · 2026-04-30T19:09:03Z

Review submitted with APPROVE. Here's a summary of what I found:

Overall: This is a well-executed extraction. The ~434-line inline page dispatch pipeline in the generated RSC entry is now a properly typed 625-line module at app-page-dispatch.ts, while the entry shrinks to a thin delegation call. All CI checks pass.

Key verification points:

The dispatch pipeline order is preserved exactly: fetch soft tags → default-export check → method policy → force-static/error setup → ISR cache read → dynamic params validation → intercept resolution → page element build → render lifecycle
The setCurrentFetchSoftTags call was actually improved — the old code used a ternary for both route handlers and pages; now each branch sets the correct tag type independently
All imports resolve correctly; the createRequestContext/runWithRequestContext names in the dispatch module are the real export names that the entry previously aliased
Test coverage is maintained: ~170 deleted lines in app-router.test.ts were generated-code string assertions replaced by delegation contract tests in entry-templates.test.ts and behavioral tests in the new app-page-dispatch.test.ts

Two non-blocking observations left as inline comments:

Defensive [...routeSegments] spreads to satisfy mutable array types — could be cleaned up by accepting readonly string[] upstream
Several type aliases duplicated between app-page-dispatch.ts and app-page-render.ts — candidates for a shared types module

github run

App Router page requests now enter a shared dispatch module after generated route matching. The generated RSC entry still describes route modules, app-shape callbacks, and request-local values, but cache reads, static generation context, dynamic param checks, intercept responses, and lifecycle rendering move into normal typed code. This keeps page orchestration out of codegen without changing the observable App Router behavior covered by the integration suite.

Copilot AI review requested due to automatic review settings April 30, 2026 05:26

Copilot started reviewing on behalf of NathanDrake2406 April 30, 2026 05:26 View session

NathanDrake2406 force-pushed the nathan/extract-app-page-dispatch branch from 7832b64 to 1a713e6 Compare April 30, 2026 05:33

Copilot AI reviewed Apr 30, 2026

View reviewed changes

NathanDrake2406 marked this pull request as draft April 30, 2026 06:52

NathanDrake2406 force-pushed the nathan/extract-app-page-dispatch branch from 1a713e6 to 2122f64 Compare April 30, 2026 18:54

NathanDrake2406 marked this pull request as ready for review April 30, 2026 18:55

NathanDrake2406 force-pushed the nathan/extract-app-page-dispatch branch from 2122f64 to 34924aa Compare April 30, 2026 19:01

ask-bonk Bot approved these changes Apr 30, 2026

View reviewed changes

NathanDrake2406 force-pushed the nathan/extract-app-page-dispatch branch from 34924aa to 888ca29 Compare May 1, 2026 08:15

chore: rerun ci

6938714

This was referenced May 1, 2026

refactor: introduce createAppRscHandler NathanDrake2406/vinext#5

Closed

refactor: introduce createAppRscHandler #999

Closed

james-elicx merged commit de00ed2 into cloudflare:main May 1, 2026
24 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(app-router): extract app page dispatch#986

refactor(app-router): extract app page dispatch#986
james-elicx merged 2 commits intocloudflare:mainfrom
NathanDrake2406:nathan/extract-app-page-dispatch

NathanDrake2406 commented Apr 30, 2026 •

edited

Loading

Uh oh!

pkg-pr-new Bot commented Apr 30, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

james-elicx commented Apr 30, 2026

Uh oh!

ask-bonk Bot left a comment

Uh oh!

ask-bonk Bot Apr 30, 2026

Uh oh!

ask-bonk Bot Apr 30, 2026

Uh oh!

ask-bonk Bot commented Apr 30, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

NathanDrake2406 commented Apr 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changed

Why

Behavior tests

Next.js references

Validation

Risk coverage

Uh oh!

pkg-pr-new Bot commented Apr 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

james-elicx commented Apr 30, 2026

Uh oh!

ask-bonk Bot left a comment

Choose a reason for hiding this comment

What I checked

Minor observations (non-blocking)

Uh oh!

ask-bonk Bot Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

ask-bonk Bot commented Apr 30, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

NathanDrake2406 commented Apr 30, 2026 •

edited

Loading

pkg-pr-new Bot commented Apr 30, 2026 •

edited

Loading